Add internal URI handling API #19073

kocsismate · 2025-07-08T21:13:52Z

No description provided.

TimWolla

Some first remarks. Did not yet look at everything.

ext/standard/url.c

TimWolla · 2025-07-09T11:43:21Z

ext/uri/php_uri.c

+	if (uri_handler_name == NULL) {
+		return uri_handler_by_name("parse_url", sizeof("parse_url") - 1);
+	}
+


Defaulting to parse_url in a new API is probably not a good idea. Instead the “legacy” users should just pass "parse_url" explicitly.

Defaulting to parse_url here works because that's the default indeed where php_uri_get_handler() is called, the other "backends" can only be used if the config is explicitly passed (not null).

The other reason why I opted for this approach is that it would be inconvenient to create and free a new zend_string when the legacy implementation is needed, and I wanted to avoid adding a known string just for this purpose, or exposing the C string based uri_handler_by_name function instead.

ext/uri/php_uri.c

TimWolla

I've looked at this again and I must say that I'm having trouble meaningfully reviewing this. It adds a large amount of code with unclear purpose and confusing (to me) naming.

ext/uri/php_uri.c

nielsdos

Preliminary review round

ext/uri/php_uri.c

ext/standard/http_fopen_wrapper.c

ext/standard/ftp_fopen_wrapper.c

ext/openssl/xp_ssl.c

ext/soap/php_http.h

ext/soap/php_http.c

nielsdos

The switch from zend_string to the pointer-length pair seems to have been a good idea

main/streams/php_stream_context.h

ext/uri/php_uri.h

ext/uri/php_uri.c

ext/soap/php_http.h

ext/soap/php_http.c

ext/openssl/xp_ssl.c

ext/filter/logical_filters.c

ext/openssl/xp_ssl.c

TimWolla · 2025-07-22T20:07:12Z

I'll try to take another look tomorrow.

ext/standard/ftp_fopen_wrapper.c

ext/openssl/xp_ssl.c

kocsismate · 2025-08-01T11:39:15Z

@nielsdos Do you see anything that I should fix before merging it? I'd like to implement some of the cleanups that we discussed

nielsdos

Looks mostly good, FWIW I agree with Tim on UNREACHABLE

ext/standard/http_fopen_wrapper.c

ext/standard/url.c

kocsismate · 2025-08-07T07:09:59Z

ext/uri/php_uri.c

-	return property_handler->read_func(internal_uri, read_mode, zv);
+	zend_result result = property_handler->read_func(internal_uri, read_mode, zv);
+
+	ZEND_ASSERT(result == FAILURE || (Z_TYPE_P(zv) == IS_STRING && GC_REFCOUNT(Z_STR_P(zv)) == 2) || Z_TYPE_P(zv) == IS_NULL || Z_TYPE_P(zv) == IS_LONG);


this assertions intends to make sure that only a new string, null, or long can be returned in case of success. I hope that you won't find it too obtrusive.

kocsismate · 2025-08-07T07:13:07Z

ext/uri/php_uri.c

 	ZEND_ASSERT(uri_handler->parse_uri != NULL);
-	ZEND_ASSERT(uri_handler->clone_uri != NULL);
-	ZEND_ASSERT(uri_handler->uri_to_string != NULL);
+	ZEND_ASSERT(uri_handler->clone_uri != NULL || strcmp(uri_handler->name, URI_PARSER_PHP) == 0);


I wanted to keep these assertions even though two handlers of the legacy parser became NULL. So I excluded this URI handler from the checks.

Assertions with exceptions to the rule are a bit iffy IMHO

Yes, I can understand your POV. Do you think it's better to get rid of the latter two special cased assertions?

kocsismate · 2025-08-07T07:15:24Z

ext/uri/php_uri.c


-	ZEND_ASSERT(uri_handler->name != NULL);
+	ZEND_ASSERT(uri_handler->name != NULL && (strlen(uri_handler->name) > 0 || strcmp(uri_handler->name, URI_PARSER_PHP) == 0));


As I renamed the legacy implementation to "", I wanted to make sure that other implementations cannot use the same handler.

Although, I think this assertion doesn't make any sense. Especially because module initialization will surely fail because of the name collision.

Huh? Why did you change the name to "" though?

Mainly because I wanted to avoid this to work:

filter_var("qwe", FILTER_VALIDATE_URL, ["uri_parser_class" => "parse_url"])

even though null is the suggested value to use for the "uri_parser_class" config. As "" is the most similar string value to null that can be stored in a uri_handlers hash table, I figured it's a better choice than parse_url.

Or do you have any better solutions in mind?

I think "" is weird from a user point of view.
I still don't think I quite understand the problem though.

ext/uri/php_uriparser.h

nielsdos

I think this is getting pretty close to being merge-able

ext/standard/http_fopen_wrapper.c

ext/standard/url.c

nielsdos · 2025-08-07T17:55:48Z

ext/uri/php_uri.c


-	ZEND_ASSERT(uri_handler->name != NULL);
+	ZEND_ASSERT(uri_handler->name != NULL && (strlen(uri_handler->name) > 0 || strcmp(uri_handler->name, URI_PARSER_PHP) == 0));


Huh? Why did you change the name to "" though?

nielsdos · 2025-08-07T17:56:07Z

ext/uri/php_uri.c

 	ZEND_ASSERT(uri_handler->parse_uri != NULL);
-	ZEND_ASSERT(uri_handler->clone_uri != NULL);
-	ZEND_ASSERT(uri_handler->uri_to_string != NULL);
+	ZEND_ASSERT(uri_handler->clone_uri != NULL || strcmp(uri_handler->name, URI_PARSER_PHP) == 0);


Assertions with exceptions to the rule are a bit iffy IMHO

kocsismate · 2025-08-08T09:29:29Z

I'll fix the CI probably tomorrow night

kocsismate requested a review from TimWolla July 8, 2025 21:13

github-actions bot added Extension: openssl Extension: filter Extension: soap Extension: standard ABI break Extension: uri labels Jul 8, 2025

TimWolla reviewed Jul 9, 2025

View reviewed changes

kocsismate force-pushed the ext-url7 branch from d2c1e8d to c8bceb8 Compare July 15, 2025 21:23

TimWolla reviewed Jul 16, 2025

View reviewed changes

ext/uri/php_uri.c Outdated Show resolved Hide resolved

kocsismate force-pushed the ext-url7 branch from c8bceb8 to fcd17da Compare July 20, 2025 21:44

github-actions bot added the Extension: reflection label Jul 20, 2025

kocsismate force-pushed the ext-url7 branch from fcd17da to 947bc64 Compare July 21, 2025 06:12

kocsismate marked this pull request as ready for review July 21, 2025 08:09

kocsismate requested review from bukka, nielsdos and DanielEScherzer as code owners July 21, 2025 08:09

nielsdos reviewed Jul 21, 2025

View reviewed changes

kocsismate mentioned this pull request Jul 21, 2025

[RFC] Add RFC 3986 and WHATWG compliant URL parsing support #14461

Open

nielsdos reviewed Jul 22, 2025

View reviewed changes

TimWolla self-requested a review July 22, 2025 20:06

nielsdos requested changes Jul 28, 2025

View reviewed changes

ext/standard/ftp_fopen_wrapper.c Show resolved Hide resolved

ext/openssl/xp_ssl.c Show resolved Hide resolved

nielsdos reviewed Aug 2, 2025

View reviewed changes

ext/standard/http_fopen_wrapper.c Show resolved Hide resolved

kocsismate force-pushed the ext-url7 branch from 12ea97b to 566c68c Compare August 7, 2025 06:58

kocsismate commented Aug 7, 2025

View reviewed changes

ext/standard/url.c Show resolved Hide resolved

kocsismate force-pushed the ext-url7 branch from 566c68c to 633b651 Compare August 7, 2025 07:08

kocsismate commented Aug 7, 2025

View reviewed changes

nielsdos reviewed Aug 7, 2025

View reviewed changes

kocsismate added 10 commits August 10, 2025 01:14

Add internal URI handling API

3ef9163

Some review comment fixes

4b06ed4

Fix tests

9dea99e

Implement review suggestions

30571fb

Change the parse_uri handler to use const char *

ea2778c

More review fixes

ab30499

Review fixes

f91d634

Try to address comments + add documentation

849971c

Fixes

e9f0a71

Fix memory leak

bd91ee6

kocsismate force-pushed the ext-url7 branch from 2543ac3 to bd91ee6 Compare August 9, 2025 23:23


		ZEND_ASSERT(uri_handler->name != NULL);
		ZEND_ASSERT(uri_handler->name != NULL && (strlen(uri_handler->name) > 0 \|\| strcmp(uri_handler->name, URI_PARSER_PHP) == 0));

Add internal URI handling API #19073

Are you sure you want to change the base?

Add internal URI handling API #19073

Uh oh!

Conversation

kocsismate commented Jul 8, 2025

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nielsdos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimWolla commented Jul 22, 2025

Uh oh!

Uh oh!

Uh oh!

kocsismate commented Aug 1, 2025

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

nielsdos left a comment •

edited

Loading